chore/no-ref: v0.3.3 code quality, CI improvements, and test coverage#6
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors the pre-commit hook to use AST-based revision extraction, typed APIs, and a per-file ChangesRelease v0.3.3 with Hook Refactor and Release Automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
stephen-kintsugi
force-pushed
the
chore/no-ref/v0.3.3-improvements
branch
from
b540e0d to
9004768
Compare
stephen-kintsugi
left a comment
stephen-kintsugi
left a comment
There was a problem hiding this comment.
Self-review walkthrough of key changes in this PR.
|
|
||
| - name: Cache virtualenv | ||
| if: inputs.install-dependencies == 'true' | ||
| uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4.3.0 |
There was a problem hiding this comment.
Shared composite action for Python/Poetry setup
Extracted from the inline steps that were duplicated between lint-and-test and auto-tag jobs. Follows the kintsugi-rules pattern: SHA-pinned actions, poetry sync -n, installer-parallel: true, and a cache key that includes the Python version for correctness across version bumps. The install-dependencies input lets the auto-tag job skip poetry install since it only needs poetry version -s.
| version: "2.1.3" | ||
| virtualenvs-create: true | ||
| virtualenvs-in-project: true | ||
| - name: Detect code changes |
There was a problem hiding this comment.
Conditional version bump and release based on package file changes
Previously, every PR (including docs and test changes) required a version bump and triggered a release on merge. Now we diff against squawk_alembic/, pyproject.toml, and .pre-commit-hooks.yaml only. The code_changed output gates both the version consistency check (line 56) and the auto-tag job (line 84). Lint and tests still run on every change.
| echo "::error::Could not parse version from pyproject.toml or __init__.py" | ||
| exit 1 | ||
| fi | ||
| PYPROJECT_VERSION=$(poetry version -s) |
There was a problem hiding this comment.
Replaced fragile grep | sed with poetry version -s and tomllib
The old version parsing (grep '^version = ' pyproject.toml | sed ...) would break if another TOML table had a version key. poetry version -s is authoritative for the current version, and tomllib (stdlib since 3.11) properly parses the TOML from git show for the main branch comparison.
|
|
||
|
|
||
| @dataclass(frozen=True, slots=True) | ||
| class RevisionInfo: |
There was a problem hiding this comment.
RevisionInfo converted to frozen dataclass
Was a manual class with __slots__ and hand-written __init__. The frozen dataclass gives us __repr__ and __eq__ for free (helps with debugging and test assertions), enforces immutability, and the type annotations on the fields document the down_revision union (str | tuple[str, ...] | None) which was previously implicit.
| pass | ||
|
|
||
|
|
||
| def _lint_file(filepath: str, migrations_path: Path, diff_branch: str | None) -> int: |
There was a problem hiding this comment.
Extracted per-file logic from main() into _lint_file()
main() was 73 lines mixing argument parsing, file iteration, SQL generation, temp file management, and subprocess calls. This extraction isolates the generate-write-lint-cleanup cycle for a single file, making main() a clean orchestration loop. The _SquawkNotFound exception bubbles up the "squawk binary missing" case, which should halt all processing (not just one file).
| except GenerateSqlError as exc: | ||
| print(str(exc), file=sys.stderr) | ||
| return 1 | ||
| except OSError as exc: |
There was a problem hiding this comment.
OSError handling for unreadable migration files
Previously, a permission error on a migration file would crash with a raw traceback. Now it produces a consistent squawk-alembic: prefixed message. The error string from OSError already includes the filepath, so we avoid duplicating it in the prefix.
|
|
||
|
|
||
| def make_result(returncode=0, stdout="", stderr=""): | ||
| return SimpleNamespace(returncode=returncode, stdout=stdout, stderr=stderr) |
There was a problem hiding this comment.
Shared test fixtures with SimpleNamespace
Consolidated duplicate repo fixtures, write_migration, fake_subprocess, and make_result from test_main.py. make_result was using type('Result', (), {...})() to create anonymous classes on every call; SimpleNamespace is the idiomatic stdlib choice for this.
| assert "some squawk warning" in captured.out | ||
|
|
||
|
|
||
| def test_squawk_failure_replaces_tmp_path_in_output(repo, capsys): |
There was a problem hiding this comment.
New test: temp file path rewriting in squawk output
Verifies that hook.py:234-235 replaces the temp file path with the original migration path in both stdout and stderr. The mock squawk process returns output containing the temp path, and we assert the user sees the original filepath instead.
| assert mock_run.call_count == 4 | ||
|
|
||
|
|
||
| def test_multiple_files_first_fails_second_still_runs(repo, capsys): |
There was a problem hiding this comment.
New test: multi-file invocation with partial failure
All previous tests passed a single file. This verifies the exit-code accumulation logic: when the first file fails (alembic error), the hook continues processing the second file and still returns exit code 1. Previously this code path had zero test coverage.
| pytest = "*" | ||
| ruff = "*" | ||
| pyrefly = "*" | ||
| alembic = "~=1.18.4" |
There was a problem hiding this comment.
Dev dependencies pinned to compatible release
Were all "*" (unpinned). Now use ~= pinned to the bugfix level from poetry.lock, so poetry update allows patches but not breaking minor/major bumps. Runtime dep squawk-cli stays at >=2.0 intentionally since consumers control their squawk version via additional_dependencies in their pre-commit config.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 22-25: The checkout step using "uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd" should not set
persist-credentials: false when later steps perform git auth-dependent commands
(e.g., the "git fetch origin main --depth=1" in the PR path and "git push origin
\"$TAG\"" in the auto-tag job); either set persist-credentials: true for those
checkout steps or re-authenticate the remote before running git commands (for
example in the job containing the fetch or push), while keeping
persist-credentials: false only for the release job checkout that has no
subsequent git operations.
In `@tests/test_main.py`:
- Around line 168-171: The test that makes a migration file unreadable sets
permissions with os.chmod then asserts main() returns 1 but only restores
permissions afterward; wrap the assert/block in a try/finally so the
os.chmod(..., 0o644) restoration runs in the finally clause to guarantee
permissions are reset even if main() or the assertion raises. Locate the
unreadable-file test where os.chmod(repo / "migrations" / "versions" /
"026_unreadable.py", 0o000) is used, keep the patch("sys.argv",
["squawk-alembic", path]) context, and move the chmod restore into the finally
block surrounding the call to main() and the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62db8de3-129e-437c-988e-fb5dc47c8a6b
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/actions/setup-python-poetry/action.yml.github/workflows/ci.ymlREADME.mdpyproject.tomlsquawk_alembic/__init__.pysquawk_alembic/hook.pytests/conftest.pytests/test_generate_sql.pytests/test_main.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_main.py (1)
153-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways restore permissions with
finallyin the unreadable-file test.If
main()or the assertion raises, the chmod reset is skipped and can leak state into later tests. Wrap the assertion block intry/finally.Suggested fix
- os.chmod(repo / "migrations" / "versions" / "026_unreadable.py", 0o000) - with patch("sys.argv", ["squawk-alembic", path]): - assert main() == 1 - os.chmod(repo / "migrations" / "versions" / "026_unreadable.py", 0o644) + unreadable = repo / "migrations" / "versions" / "026_unreadable.py" + os.chmod(unreadable, 0o000) + try: + with patch("sys.argv", ["squawk-alembic", path]): + assert main() == 1 + finally: + os.chmod(unreadable, 0o644)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_main.py` around lines 153 - 171, In test_unreadable_migration_file, ensure file permissions are always restored by wrapping the call to main() and the assertion in a try/finally: after setting 0o000 on the migration file call main() (and assert its return) inside try, and in finally call os.chmod(..., 0o644) to reset permissions; reference the test function name test_unreadable_migration_file and the main() invocation to locate where to add the try/finally.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
9-11: ⚡ Quick winUse a stable concurrency key per branch/PR instead of commit SHA.
Line 10 uses
github.shafor push events, which makes each run’s group unique and preventscancel-in-progressfrom cancelling older in-flight runs on the same branch.Suggested diff
concurrency: - group: ci-${{ github.head_ref || github.sha }} + group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 9 - 11, The concurrency group uses a per-commit SHA which prevents cancel-in-progress from grouping runs by branch/PR; update the concurrency.group expression to use a stable branch/PR identifier by replacing github.sha with github.ref_name and keep the existing head-ref fallback so it reads something like: use github.head_ref || github.ref_name inside the concurrency.group template (the change is to the concurrency `group` key in the workflow).squawk_alembic/hook.py (1)
241-242: 💤 Low valueUse exception chaining with
from Noneto suppress the original traceback.When re-raising as a different exception type intentionally, explicit chaining clarifies that the
FileNotFoundErroris being transformed, not an error in the handler itself.Suggested fix
except FileNotFoundError: - raise _SquawkNotFound + raise _SquawkNotFound from None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@squawk_alembic/hook.py` around lines 241 - 242, In the except FileNotFoundError handler where you currently do "raise _SquawkNotFound", change the re-raise to use explicit exception chaining suppression by raising "_SquawkNotFound from None" so the original FileNotFoundError traceback is suppressed; locate the except block that catches FileNotFoundError and update the raise statement accordingly for the _SquawkNotFound exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/test_main.py`:
- Around line 153-171: In test_unreadable_migration_file, ensure file
permissions are always restored by wrapping the call to main() and the assertion
in a try/finally: after setting 0o000 on the migration file call main() (and
assert its return) inside try, and in finally call os.chmod(..., 0o644) to reset
permissions; reference the test function name test_unreadable_migration_file and
the main() invocation to locate where to add the try/finally.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 9-11: The concurrency group uses a per-commit SHA which prevents
cancel-in-progress from grouping runs by branch/PR; update the concurrency.group
expression to use a stable branch/PR identifier by replacing github.sha with
github.ref_name and keep the existing head-ref fallback so it reads something
like: use github.head_ref || github.ref_name inside the concurrency.group
template (the change is to the concurrency `group` key in the workflow).
In `@squawk_alembic/hook.py`:
- Around line 241-242: In the except FileNotFoundError handler where you
currently do "raise _SquawkNotFound", change the re-raise to use explicit
exception chaining suppression by raising "_SquawkNotFound from None" so the
original FileNotFoundError traceback is suppressed; locate the except block that
catches FileNotFoundError and update the raise statement accordingly for the
_SquawkNotFound exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea0b3248-f919-4d6a-93e2-396177ea9956
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/actions/setup-python-poetry/action.yml.github/workflows/ci.ymlREADME.mdpyproject.tomlsquawk_alembic/__init__.pysquawk_alembic/hook.pytests/conftest.pytests/test_generate_sql.pytests/test_main.py
✅ Files skipped from review due to trivial changes (1)
- README.md
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9dc608f. Configure here.
2 participants
Summary
We haven't touched this repo in a while, so we ran a comprehensive review to address accumulated tech debt and bring CI/CD up to Kintsugi standards.
hook.py: extracted_lint_file()frommain(), convertedRevisionInfoto frozen dataclass, added type hints and docstrings to all functions, addedOSErrorhandling for unreadable migration filestest_generate_sql.pyfor direct unit tests, sharedconftest.pyfor fixtures, new tests for missing squawk binary, multi-file invocation, and temp path rewritingpoetry version -sandtomllibreplacing fragilegrep | sed, timeout-minutes on all jobs~=) from lock file versions, addedpytest-covpoetry updateto pull latest compatible dependency versionsJira
N/A
Tests
49 tests passing, 97% line coverage on
squawk_alembic/hook.py. Pre-commit hooks (ruff, pyrefly, formatting) all green.Document Checklist
Note
Medium Risk
Medium risk because it refactors the core
squawk_alembichook execution path and adjusts CI release automation; behavior changes are mostly covered by expanded tests but could affect migration linting and tagging in edge cases.Overview
Bumps the package to v0.3.3 and refreshes dev tooling by pinning dev dependencies (plus adding
pytest-cov) and updating the lockfile.Refactors
squawk_alembic/hook.pyto make linting more robust and testable: convertsRevisionInfoto a frozen dataclass with type hints, extracts per-file lint logic into_lint_file(), adds clearer handling for unreadable migration files, missingsquawk, and rewrites squawk output paths from temp files back to the original migration path.Overhauls CI to be more deterministic and release-safe by introducing a reusable Python/Poetry setup composite action (with
.venvcaching), SHA-pinning actions, adding concurrency/timeouts, and gating version checks, auto-tagging, and releases on detected changes to distributable package files; version parsing is switched fromgreptopoetry version/tomllib.Expands test coverage with shared fixtures (
tests/conftest.py) and new unit tests forgenerate_sql, plus additional main-path tests for multi-file runs and error cases.Reviewed by Cursor Bugbot for commit a65eaa0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores
Refactor
Tests